Reintroduce reflective REPL pprint call. #24353
Open
+38
−3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bishabosha
added
the
backport:nominated
Contributor
|
Contributor
|
Looks good to me, need this to merge first so I can rebase #24298 on top |
lihaoyi
approved these changes
Nov 9, 2025
Contributor
|
@bishabosha how are you testing this? |
Contributor
Scala CLI v1.10.0 handles the new REPL artifact. |
Member
Author
|
I test by publishing the binary locally with java -cp $(cs fetch -p org.scala-lang:scala3-repl_3:3.8.0-RC1-bin-SNAPSHOT) dotty.tools.repl.Main -cp $(cs fetch -p org.scala-lang:scala3-repl_3:3.8.0-RC1-bin-SNAPSHOT)although @hamzaremmal is working on either an alternative or replacement PR that should hide pprint from visible class path I think |
Contributor
|
@Gedochao I'm getting an error running |
Contributor
|
same failure happens on d47c0c6 as well, so even a clean |
Member
I've been talking with Jamie about this PR offline and it is just lacking a comment that explains how all of this works. |
Contributor
|
@bishabosha I'm getting some errors trying the command you gave. Did you have to run anything else to make it work? |
Contributor
|
Turns out I needed |
Member
Author
|
I also heard that apparently you should use |
bishabosha
force-pushed
the
reintroduce-reflective-pprint
branch
2 times, most recently
from
5984686 to
6956183
Compare
The class name changed, but the reflective call is still necessary for deeply nested objects to be formatted correctly. Includes a comment describing why the reflection is needed.
bishabosha
force-pushed
the
reintroduce-reflective-pprint
branch
from
6956183 to
23b7ce1
Compare
Member
Author
|
@hamzaremmal I have pushed a comment explaining the situation with class loaders why reflection is necessary |
Member
Author
|
There is still potentially an alternative to avoid reflection but would require delegating all the classes pprint cases about to the root jvm classloader, (while hiding the rest of the classpath) and not instrumenting those ones also. Could be considered more complex then using reflection |
Contributor
|
@hamzaremmal I noticed you removed the shading from fansi/pprint. Was that intentional? Without shading it opens up two potential conflicts:
If we don't want to shade them, we would need to provide classloader isolation for both of these scenarios, e.g. the REPL provides an artifact that loads its classes from a jar embedded as a resource file, and then compiles snippets without those classes included |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The class name changed, but the reflective call is still necessary for deeply nested objects to be formatted correctly.
@tgodzik @Gedochao for 3.8 backport recommendation
./cc @lihaoyi